chore: reconcile downstream version dependencies on job success#1085
chore: reconcile downstream version dependencies on job success#1085adityachoudhari26 merged 2 commits intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughIntroduces a new database query to fetch distinct release targets for a deployment-resource pair across environments, with supporting generated code. Adds a downstream dependency dispatch routine in the job controller that queries upstream jobs and enqueues release evaluations for affected downstream deployments when a job succeeds. Also adds deterministic ordering to an existing deployment dependency query. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
There was a problem hiding this comment.
Pull request overview
This PR ensures deployment-version dependency gates get re-evaluated promptly by enqueueing downstream desired-release reconciliations when an upstream job becomes successful (i.e., when the “current release” for a resource can change).
Changes:
- Trigger downstream dependency fan-out from
PostgresSetter.UpdateJobwhen the job status transitions tosuccessful. - Add
dispatchDependencyDownstreamTargetsto find downstream deployments (via version dependencies) and enqueue desired-release evaluations for matching (deployment, environment, resource) release targets. - Add a new sqlc query
GetReleaseTargetsForDeploymentAndResource(and generated Go) to enumerate downstream release targets for a specific deployment/resource pair.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| apps/workspace-engine/svc/controllers/jobdispatch/setters_postgres.go | Calls the new downstream-dispatcher when a job becomes successful. |
| apps/workspace-engine/svc/controllers/jobdispatch/dispatch_dependency_downstream.go | Implements downstream dependency discovery + enqueueing desired-release eval work items. |
| apps/workspace-engine/pkg/db/queries/computed_resources.sql | Adds a query to list all release targets for a (deployment, resource) across environments. |
| apps/workspace-engine/pkg/db/computed_resources.sql.go | sqlc-generated Go for the new computed_resources query. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| return fmt.Errorf( | ||
| "get release targets for downstream deployment %s: %w", | ||
| downstreamDepID, |
There was a problem hiding this comment.
The error format string uses %s with downstreamDepID (a uuid.UUID), which will produce a malformed %!s(uuid.UUID=...) message. Use downstreamDepID.String() (or %v) so the deployment ID is correctly rendered in errors/logs.
| downstreamDepID, | |
| downstreamDepID.String(), |
| downstreamDeps, err := queries.GetDeploymentsWithVersionsDependingOn(ctx, release.DeploymentID) | ||
| if err != nil { | ||
| return fmt.Errorf("get deployments with versions depending on: %w", err) | ||
| } |
There was a problem hiding this comment.
GetDeploymentsWithVersionsDependingOn is not scoped to the upstream job's workspace, but you enqueue downstream desired-release evals using the upstream wsID for every item. If a deployment version dependency can reference deployments across workspaces (schema doesn't appear to prevent it), this could enqueue work items into the wrong workspace / leak IDs. Consider scoping the query by workspace (e.g., add workspace_id param and filter on deployment_version.workspace_id or join deployment) or fetching each downstream deployment's workspace ID and using that when enqueueing.
| var params []events.DesiredReleaseEvalParams | ||
|
|
||
| for _, downstreamDepID := range downstreamDeps { | ||
| rts, err := queries.GetReleaseTargetsForDeploymentAndResource( | ||
| ctx, | ||
| db.GetReleaseTargetsForDeploymentAndResourceParams{ | ||
| DeploymentID: downstreamDepID, | ||
| ResourceID: release.ResourceID, | ||
| }, | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf( | ||
| "get release targets for downstream deployment %s: %w", | ||
| downstreamDepID, | ||
| err, | ||
| ) | ||
| } | ||
| for _, rt := range rts { | ||
| params = append(params, events.DesiredReleaseEvalParams{ | ||
| WorkspaceID: wsIDStr, | ||
| ResourceID: rt.ResourceID.String(), | ||
| EnvironmentID: rt.EnvironmentID.String(), | ||
| DeploymentID: rt.DeploymentID.String(), | ||
| }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This does one DB query per downstream deployment (GetReleaseTargetsForDeploymentAndResource inside the loop). If a deployment has many downstream dependents, this becomes an N+1 query pattern. Consider adding a single sqlc query that accepts downstream_deployment_ids (uuid[]) + resource_id and returns all (deployment, environment, resource) targets in one round-trip, then build params from that result.
| var params []events.DesiredReleaseEvalParams | |
| for _, downstreamDepID := range downstreamDeps { | |
| rts, err := queries.GetReleaseTargetsForDeploymentAndResource( | |
| ctx, | |
| db.GetReleaseTargetsForDeploymentAndResourceParams{ | |
| DeploymentID: downstreamDepID, | |
| ResourceID: release.ResourceID, | |
| }, | |
| ) | |
| if err != nil { | |
| return fmt.Errorf( | |
| "get release targets for downstream deployment %s: %w", | |
| downstreamDepID, | |
| err, | |
| ) | |
| } | |
| for _, rt := range rts { | |
| params = append(params, events.DesiredReleaseEvalParams{ | |
| WorkspaceID: wsIDStr, | |
| ResourceID: rt.ResourceID.String(), | |
| EnvironmentID: rt.EnvironmentID.String(), | |
| DeploymentID: rt.DeploymentID.String(), | |
| }) | |
| } | |
| } | |
| rts, err := queries.GetReleaseTargetsForDeploymentsAndResource( | |
| ctx, | |
| db.GetReleaseTargetsForDeploymentsAndResourceParams{ | |
| DeploymentIds: downstreamDeps, | |
| ResourceID: release.ResourceID, | |
| }, | |
| ) | |
| if err != nil { | |
| return fmt.Errorf("get release targets for downstream deployments: %w", err) | |
| } | |
| params := make([]events.DesiredReleaseEvalParams, 0, len(rts)) | |
| for _, rt := range rts { | |
| params = append(params, events.DesiredReleaseEvalParams{ | |
| WorkspaceID: wsIDStr, | |
| ResourceID: rt.ResourceID.String(), | |
| EnvironmentID: rt.EnvironmentID.String(), | |
| DeploymentID: rt.DeploymentID.String(), | |
| }) | |
| } |
| // Deployment-version dependency gates only react to "current release" | ||
| // changes, which only happen when a job becomes successful. | ||
| if status == oapi.JobStatusSuccessful { | ||
| if err := dispatchDependencyDownstreamTargets(ctx, s.Queue, jobIDUUID); err != nil { | ||
| return fmt.Errorf("dispatch dependency downstream targets: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
New behavior is introduced here (enqueueing desired-release reconciliations for downstream deployment-version dependencies when a job becomes successful), but there doesn't appear to be any test coverage asserting the downstream fan-out and enqueue behavior. Please add a unit/integration test that exercises a job status transition to successful and verifies the expected desired-release work items are enqueued for downstream deployments on the same resource.
fixes #1079
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes